-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: FromUTXOs #45
Conversation
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
=======================================
Coverage 86.04% 86.05%
=======================================
Files 26 26
Lines 2817 2862 +45
=======================================
+ Hits 2424 2463 +39
- Misses 270 271 +1
- Partials 123 128 +5
Continue to review full report at Codecov.
|
txinput.go
Outdated
|
||
// FundIteration contains information relating to the current fund. Its fields are intended | ||
// for use with tx.From(...). | ||
type FundIteration struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this to just Fund I think - does that clash with anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, but I'm concious of bt.Fund
being confused with tx.Input
. What do you reckon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should just return a bt.Input, any reason it wouldn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, it just means the the implementor has to build the Input themselves, being, parse the lockingscript into a *bscript.Script, and manually add the input tx id, instead of just being able to call the From
helper function.
I'll change it over here now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a NewInputFrom
helper func to dodge this, let me know what you think.
txinput.go
Outdated
// containing relevant input information, and a bool informing if the retrieval was successful. | ||
// | ||
// It is expected that the boolean return value should return false once funds are depleted. | ||
type FundIteratorFunc func() (*FundIteration, bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this will most likely be looking up funds from a data store, add ctx as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would FundGetterFunc be a better name for this do you think as it explains more what it's doing, getting a single fund.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought since we're requesting n funds from a collection without any identifying args, leaving state management up to the implementor, iterator was better to describe it.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye the more I read it as FundGetter the more I agree actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah as each call will get a fund / input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one small comment to fix, but looks good
txinput.go
Outdated
"fmt" | ||
|
||
"github.com/libsv/go-bk/crypto" | ||
|
||
"github.com/libsv/go-bt/v2/bscript" | ||
) | ||
|
||
// ErrNoInput signals the InputGetterFunc has reached the end of its input. | ||
var ErrNoInput = errors.New("no remainings inputs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 'remaining'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorted there
txinput.go
Outdated
// NewInputFrom builds and returns a new input from the specified UTXO fields, using the default | ||
// finalised sequence number (0xFFFFFFFF). If you want a different nSeq, change it manually | ||
// afterwards. | ||
func NewInputFrom(prevTxID string, vout uint32, prevTxLockingScript string, satoshis uint64) (*Input, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these functions go into the inputs.go file instead of here? i think what we were going for was to try an minimise the number of ways users could do things. so in v1 the user had to create the input himself and then add that input to the tx, whereas with v2 the point is to use the .From() method to create the input as part of the tx straight away since having an input alone isn't much use is it? maybe we can keep this func but just put it in inputs.go and make in private? what do you guys think? @theflyingcodr @Tigh-Gherr ? that was the reasoning behind making tx.addInput private as well especially since it confused a couple devs into using it the wrong way in the past...
txinput.go
Outdated
// return bt.NewInputFrom(utxos[i].TxID, utxo[i].Vout, utxos[i].Script, utxos[i].Satoshis), true | ||
// } | ||
// }()) | ||
func (tx *Tx) FromInputs(ctx context.Context, fq *FeeQuote, next InputGetterFunc) (err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh this is good shit, i like it 👍
txinput.go
Outdated
// relevant input information, and an err informing any retrieval errors. | ||
// | ||
// It is expected that bt.ErrNoInput will be returned once the input source is depleted. | ||
type InputGetterFunc func(ctx context.Context) (*Input, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see this went thought a few iterations haha but i still think it may be able to go for 1 more iteration to make it little clearer - let's discuss on monday 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like where this is going actually, but i have a much better idea of how i would like this to work now and it follows the reasoning that we were discussing about potentially making inputs and outputs private that are used in a tx - since (unless someone can convince me otherwise) we will never have an input or output that isn't part of a tx and it just creates confusion, as we've seen previously, when separating them.
1st: we should probably return an array here instead of just one (input) since many times you will have an array of utxos to choose from and then we can add functionality to choose different utxos and stuff. plus of the utxogetter involved i/o then we'd want to minimise that and create a bulk call or something like that which we've done many times before.
2nd: it doesn't really feel accurate to say input getter since we're not getting the inputs, we're getting the utxos and creating the input... i think it would be much better to change this to UTXOGetterFunc and have it return an array of utxos and then we internally create the input from the utxo(s) and add them to the tx.
i'm still unsure whether we should still make the inputs/outputs private (ideally we could just leave them public to retain flexibility for users that know what they're doing but to remove all functions that imply separation between inputs/outputs and txs - for example the NewInput/Ouput funcs)...
thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add satoshis as a param and return array instead of only 1 element
- create new data type: UTXO with the following 4 fields: prevTxID string, vout uint32, prevTxLockingScript string, satoshis uint64
- refactor tx.From() to take UTXO instead of 4 params
- rename InputGetterFunc to UTXOGetterFunc and return array of UTXOs
txinput.go
Outdated
// relevant input information, and an err informing any retrieval errors. | ||
// | ||
// It is expected that bt.ErrNoInput will be returned once the input source is depleted. | ||
type InputGetterFunc func(ctx context.Context) (*Input, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like where this is going actually, but i have a much better idea of how i would like this to work now and it follows the reasoning that we were discussing about potentially making inputs and outputs private that are used in a tx - since (unless someone can convince me otherwise) we will never have an input or output that isn't part of a tx and it just creates confusion, as we've seen previously, when separating them.
1st: we should probably return an array here instead of just one (input) since many times you will have an array of utxos to choose from and then we can add functionality to choose different utxos and stuff. plus of the utxogetter involved i/o then we'd want to minimise that and create a bulk call or something like that which we've done many times before.
2nd: it doesn't really feel accurate to say input getter since we're not getting the inputs, we're getting the utxos and creating the input... i think it would be much better to change this to UTXOGetterFunc and have it return an array of utxos and then we internally create the input from the utxo(s) and add them to the tx.
i'm still unsure whether we should still make the inputs/outputs private (ideally we could just leave them public to retain flexibility for users that know what they're doing but to remove all functions that imply separation between inputs/outputs and txs - for example the NewInput/Ouput funcs)...
thoughts?
txinput.go
Outdated
for i, utxo := range pvsTx.Outputs { | ||
utxoPkHASH160, err := utxo.LockingScript.PublicKeyHash() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if bytes.Equal(utxoPkHASH160, crypto.Hash160(matchPK)) { | ||
if err := tx.From(pvsTx.TxID(), uint32(i), utxo.LockingScriptHexString(), utxo.Satoshis); err != nil { | ||
if err := tx.From(&UTXO{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just have?
TxID: pvsTx.TxID(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TxID
func hashes, reverses bytes then hex encodes every call. I figured there's no point in doing that work multiple times, being each iteration, since the result never changes, esp since the previous tx could have thousands of outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart 👍 (a comment there to explain would be great actually!)
utxo.go
Outdated
type UTXO struct { | ||
TxID string | ||
Vout uint32 | ||
LockingScript string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm you think this should be a *bscript.Script like the inputs and outputs? also the TxID be a byte array? https://github.com/libsv/go-bt/blob/master/input.go#L31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with both of those, just as this is going to be used as params for tx.From
, is there a reason the params for that function were both strings instead of []byte
and *bscript.Script
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one suggestion around deficit name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A utility func to attempt to abstract the funding flow of a tx away from the user. All the user has to do is provide an interator func.
Example usage:
After completion, the tx is then ready for its change to be added, and ultimately signed.